perf: incremental memory optimization across Maka - #5153

Reviewed across six dimensions: dependency patches, lifetime/ownership, Rust WebRTC, text truncation, transport framing, Desktop/UI. Differential fuzzing confirmed the 13 rewritten truncation functions and the four framing changes are behaviour-preserving; cargo test webrtc_direct passes. Most of the 64 items are correct equivalent rewrites, and several fix real retention (Path.remainder, the AppShell session→epoch map, the Promise.race reaction in session continuity, zod's lastCtx).

The findings below cannot be fixed at a single line. Line-level ones are filed as inline comments.

P1 — Collapsed reasoning bodies leave the DOM without a decision

packages/ui/src/astryx-chat-reasoning.tsx

Collapse was grid-template-rows: 0fr + overflow: hidden, not display: none. Content stayed in the accessibility tree and stayed findable with Cmd+F. Item 22 removes it until first expansion, so never-opened thinking text is unreachable for screen readers and find-in-page. Streaming collapsed blocks are worse: previewText is gated on !isStreaming, so they expose only "Thinking".

The PR resolved this by editing an existing test to click the header before reading .maka-chat-reasoning-content. That test previously read the text without clicking — the DOM contract changed and the change was absorbed rather than raised.

Per CONTRIBUTING, removing a capability or changing an interaction needs a decision in an issue or Discussion first. Either open one stating the a11y/find-in-page cost this buys, or keep the subtree mounted with hidden="until-found" — which preserves find-in-page but not the memory this item is after.

P1 — Identity comparison for idempotent replay now has two authorities

packages/runtime-host/src/server/message-coordinator.ts, packages/runtime-host/src/server/root-admission-owner.ts

Items 04 and 09 are described as storing less. They also replace the conflict predicate: isDeepStrictEqual(payload) / messageContentsEqual(content) become digest comparison. Getting this wrong either duplicates a delivery or rejects a legitimate resend as operation_conflict — a reconnect-and-resend path.

The two rule sets do line up field for field today. But messageContentsEqual and normalizeMessageContent are written separately in events.ts, and nothing binds them. Editing either silently breaks idempotent replay with no failing test.

Make messageContentsEqual call messageContentDigest on both sides so there is one rule. That also removes the superseded predicate rather than leaving both.

P2 — Releasing a reference and closing in order are treated as one primitive

Two items make the same mistake in opposite directions.

Item 06, packages/runtime/src/stream-graph-coordinator.ts:1453 — #retireDriver awaits driver.stopTask, the operator teardown I/O, with no timeout and no abort (beginDrain's abort only reaches driver.abortController). Its only production caller is prepareFreshAgentGraphEpoch, so every new Turn in graph/swarm mode now starts behind the previous epoch's shutdown. Hang one operator in teardown and the next Turn cannot start; on main the epoch advances regardless.

Item 13, native/runtime-host-peer/src/webrtc_direct/lifetime.rs:48 — close() cancels before peer_connection.close(), cutting off workers still inside drive_data_channel and discarding both the per-substream outgoing queue and the up-to-5s drain_data_channel flush. On main, close and the workers ran concurrently, so drain could complete.

Dropping a reference is synchronous, has no side effects, and can always happen immediately. Closing in order is asynchronous, does I/O, and can fail or hang. Item 06 waited for a close under the name of releasing; item 13 cancelled under the name of closing. Both are one line, but fixing them separately leaves the distinction unstated.

void this.#retireDriver(driver) works for 06 — Promise.allSettled already absorbs the errors — or bound the stopTask wait. For 13, move the cancel after peer_connection.close().await; Drop should keep cancelling first, since that path has no graceful-close semantics.

P2 — A shared invariant is rederived at 13 call sites

Items 42–63 all rest on the same fact: a code point encodes to at least one UTF-8 byte, so collecting budget + 1 UTF-16 units before slicing by bytes is safe. Every derivation is correct — but each is local.

The cost is already visible: tool-output.ts guards degenerate budgets to preserve Buffer.subarray's negative-end semantics, web-fetch-tool.ts uses the same technique without the guard. Unreachable today because the budget is a constant; it is what rederiving the same reasoning 13 times produces.

text-line-window.ts is the counter-example — it merges two duplicated line-window implementations into one authority. Same treatment for a boundedPrefixByBytes(text, maxBytes) would let most of these call sites shrink to one call. A smaller instance: use-composer-history.ts fixes a non-lazy useRef, use-composer-attachments.ts keeps one; a shared useLazyRef covers both.

P2 — Canonical-base64 validation and length measurement are separable

Buffer.byteLength(s, 'base64') equals the decoded length only for canonical input — Node computes (len - padding) * 3 >>> 2 without checking the alphabet, whitespace, or a multiple-of-four length. All four call sites validate first, so item 55 is correct as written.

Nothing in the signature carries that precondition, and the input is peer-controlled. A future call site that skips validation lets a peer pass SESSION_TRANSCRIPT_PAGE_MAX_BYTES while decoding to a different size. A canonicalBase64ByteLength(value) that validates and measures together removes the gap and drops one duplicated regex.

P2 — The PR's central premise is an unspecified engine detail, stated five times

Five sites rely on structuredClone(<string>) to detach a slice from an oversized backing string. That is a V8 value-serializer behaviour, not a language guarantee, and items 23/33/35/36/37/39 all depend on it.

Right now the same justification is repeated in five comments with no measurement. One detachString(s) helper, explained once, plus a single retained-size heap snapshot in the PR body would make the premise checkable.

P2 — The tests for these changes exist in this branch's history

44bc559f6 removes 65 test files and 11,509 lines. That includes the behaviour-asserting ones this review would otherwise ask for: zod-memoizer-lifetime.test.ts, webrtc_direct/lifecycle_tests.rs, request-customization-abort.test.ts, goal-token-lifetime.test.ts, stream-graph-coordinator-retirement.test.ts, computer-use-session-lifetime.test.ts, file-read-line-window-allocation.test.ts, edit-diff-window-allocation.test.ts.

Dropping the allocation probes is reasonable — they are unstable across Node versions and Electron, which the earlier make prompt heap checks portable to Electron commit already shows. The eight above assert behaviour, not allocation counts, and fail on the old code. Restoring them from 44bc559f6^ costs far less than rewriting them.

P3 — Three body entries do not match the diff

• 07 describes connection-lifetime promise waits becoming removable close listeners. The diff for session-transcript.ts and session-transcript-pager.ts contains only the Buffer.byteLength change.
• 31/32 describe sharing frozen snapshots and completed prompt strings. The actual change bounds code-point array allocation.
• 27 omits that applyServiceRelease no longer notifies onSessionInvalidated for locally unknown sessions.

In a body that enumerates 64 items, mismatches cost the reader confidence in the other 61.

P3 — Three behaviour fixes are filed as allocation work

Item 25 restores cancellation through request customization, item 27 adds the known gate, and the two identity changes above alter a predicate. Each is a fix worth having and worth its own commit and body entry; under "reduce redundant allocations" they get skimmed.

P3 — The three new patches have no upstream exit

patches/README.md entries reference upstream issues elsewhere (See #2978, See #1967 / #1976, See #3446). The zod, MCP and pi-tui entries have none, and their stated exit conditions describe upstream behaviour changes that nobody upstream has been asked for. The zod parse-state leak and the MCP settled-observer retention are general bugs, not Maka-specific needs.

Re-reviewed at b2c6333f4, fresh, against the previous round. Five follow-up commits land the two P1 inline findings and four of the smaller ones, each with a regression test that I confirmed fails on the pre-follow-up source. Of the seven declined items I checked on the code rather than the argument: five rebuttals are correct and two earlier findings were overstated — the replay-identity P1 and the WebRTC half of the shutdown P2 do not describe a present defect, and I withdraw them. One new issue falls out of the cold-summon fix itself and is filed inline.

Status of the previous round

On body 2: messageContentsEqual and messageContentDigest do agree on every supported input. AttachmentRef, DirectoryReference, QuoteRef and InlineReference are closed shapes declared through defineObjectShape, and every field of each is compared by the old predicate and kept by normalizeMessageContent; the empty-array-vs-undefined collapse matches on attachments, quotes and directoryReferences, and is deliberately absent on inlineReferences in both. No input separates them, so this is a maintenance preference, not a replay defect. The same applies to the digest change in root-admission-owner.ts.

On body 3b: muxer.rs:148 calls lifetime.cancellation.cancel() before lifetime.close(), so moving the cancel inside close() changes nothing on the production path. The residual delta — workers aborted before peer_connection.close(), where on main there was no cancellation at all — costs at most the tail of an outgoing substream at connection teardown, which resumable-peer-stream retransmits. P3, not blocking.

P2 — Never-expanded reasoning is unreachable to find-in-page and screen readers

Reachability class ① — open a restored transcript and search it.

packages/ui/src/astryx-chat-reasoning.tsx:149 renders {isExpanded || hasExpanded ? children : null}. The previous collapse was grid-template-rows: 0fr plus overflow: hidden, which keeps the subtree in the accessibility tree and findable with Cmd+F. A reasoning block that was never opened in this session — which is every block in a transcript restored from history, exactly where searching matters — now has no text to find.

The process half of the earlier finding does not hold, and I withdraw it: CONTRIBUTING.md:30 reserves the dev@ list for "project direction, governance, and material product decisions" and says implementation-level decisions may live in the pull request. This is an implementation decision.

The accessibility cost stands on its own and is not mitigated by the header staying keyboard-operable — a reader has to know the text is there to expand it. hidden="until-found" restores find-in-page but not the memory, so it is not a free fix. What would settle it is a number: item 22 claims a DOM-residency win with no measurement, and the size of that win is what decides whether the trade is worth making. Either attach one, or scope the deferral to blocks above some length so short reasoning stays searchable.

P2 — Four of the eight behaviour tests are still missing, and they cover the least-verified changes

Reachability class ② — the changes they would cover are cancellation, shutdown and recovery paths.

44bc559f6 remains as pushed. The follow-up added targeted coverage in four existing suites — Graph retirement, BotRegistry SDK failure, WorkHub cold summon, CU session cleanup — and I confirmed all four fail on the pre-follow-up source. The author is right that the file-read and edit-diff files were allocation probes and that some of the others carried GC/WeakRef assertions.

What is left uncovered is the part with the least independent evidence:

• 5af4f263b restores cancellation through request customization (request-customization-fetch.ts:51). This is a behaviour fix on an abort path with no test that fails without it.
• 1789afeac gates the Goal token cache on the control lease (goal-coordinator.ts:165) and clears it on removal.
• The zod patch's open/handoff handling, whose only evidence is the counterexample script in the inline thread.
• lifetime.rs cancellation ordering, whose Rust lifecycle test was removed.

Each is one test in an existing suite, in the same shape as the four that landed. Cherry-picking from 44bc559f6^ is cheaper than rewriting, and the GC assertions can be dropped from the restored files.

Verification

Clean npm ci, all affected workspaces built; runtime 98, runtime-host 108, computer-use 117, ui 430, desktop main 2468 green; lint and format clean. Red-on-base (production file reverted alone): graph epoch handover, BotRegistry SDK failure, both WorkHub cold-summon tests, CU known gate — all five fail before the follow-up. Also re-derived with no defect found: the four Buffer.byteLength(x, 'base64') sites, sliceLineByBytes / truncateToolOutput at degenerate budgets, a 120 000-case base-vs-head differential fuzz of createEditUnifiedDiff / createUnifiedDiff (zero diffs), the peer-mesh/node.ts wake rewrite, the app-shell.tsx hydration fence, and the WeChat iterator completion protocol.

Nothing else at P0–P2. With the three above addressed — or, for the first two, answered with a measurement — this is ready to approve.

Third round on 2bb009541. Two of the three items are settled; the new Rust test does not pass on macOS.

1. Reasoning DOM residency — accepted. The measurement is there and the method is sound for what it claims: identical 376,472-byte viewmodel, real TurnView, separate renderer processes, post-GC heap and DOM element counts (10,839,440 → 4,895,284 B; 5,020 → 679 elements), explicitly scoped to component residency rather than RSS or a typical workload. Retracting the earlier "not found in either version" probe and stating plainly that find-in-page now requires expansion is the right call, and it makes the trade-off reviewable instead of disputed. hidden="until-found" is correctly rejected: it only works on a mounted subtree, which is exactly the residency this change removes, and the collapsed panel is display-hidden, so screen readers never reached that content in either version either. No length threshold needed.

2. Restored coverage — three of four confirmed, lifetime.rs not. Verified by reverting each production change and rebuilding: request-customization-fetch.ts:51 (both new cases red — the signal assertion, and the HTTP one timing out at ~2.1 s as described), goal-coordinator.ts:165 (baseline 30 instead of 120, exactly as claimed), and the zod handoff restore (both ESM and CJS red; removing the try/finally alone also turns ESM red, so throw-recovery is covered too). connection_teardown_releases_a_backpressured_writer fails on macOS 26.6 arm64, 3/3 runs, at tests.rs:144 — the 4 MiB write_all returns Ok(Ok(())) inside 100 ms, so the precondition that the writer is backpressured never holds and the teardown contract is never exercised. CI only runs cargo test on ubuntu-24.04 (runtime-host-peer-admission.yml:56), so it stays green there while failing for every macOS contributor. Fix: instead of asserting one fixed-size write stalls, write bounded chunks in a loop until a write actually fails to complete within the window (or drive the write in a task and assert only the release), so the test states the contract without depending on a host's SCTP buffer size.

3. WorkHub background throttling — accepted, rejection is correct. Electron's Page visibility section says a window created with show: false gets initial visibility state visible despite being hidden, and this path does not set paintWhenInitiallyHidden: false, so the cold-summon bootstrap is not a backgrounded document. The code confirms the asymmetry with requestProgress independently: the progress card signals readiness from a double requestAnimationFrame (workhub-progress-card.tsx:57), which a suspended RAF would deadlock — that is why #5114 disabled throttling at :365 — whereas cold-summon readiness comes from a mount effect (workhub-surface-switch.tsx:33), which throttling cannot stall. The 337.89 vs 333.84 ms A/B is consistent with that and no toggle is warranted.

Fourth round on 13f27da75. The one open item is fixed: git diff 2bb009541..13f27da75 --stat is webrtc_direct/tests.rs only, +52/-25, no production change. The test now states the contract instead of a host's buffer size, and it passes here.

Backpressure is now observed, not assumed. The loop at native/runtime-host-peer/src/webrtc_direct/tests.rs:146 writes and flushes 8 KiB chunks until one operation is still pending after 100 ms, and carries that future through teardown — it is not a bigger fixed write or a longer sleep. Instrumenting the loop on this machine shows it needs 6,643,712 bytes before a write stays pending, identically for both explicit_close arms, which is exactly why the previous fixed 4 MiB write_all returned Ok inside the window here.

It cannot hang. Both guards are checked before each attempt: sent < 64 MiB (:147) and a 5-second setup_deadline (:151), so a host that never backpressures fails with a named panic rather than spinning. Every await after that is inside a 5-second timeout.

Dropping the pending write's error assertion is the right call. Bytes already queued can legitimately complete during close, so let _ = ...timeout(..., &mut writing) (:180) keeps the part that is actually contractual — the operation must settle, not stay parked — and the observable claim moves to the post-teardown write at :184, which must fail. That is a stronger statement than the old one and does not encode a close/cancel ordering.

Verified on macOS 26.6 arm64 (cargo 1.98.0): cargo test --locked --lib webrtc_direct passed 5/5 consecutive runs (6 tests each), and cargo test --locked for the crate passed 39/39. The probe edit above was reverted before those runs; the tree matches 13f27da75. No P0–P2 findings, and nothing at P3.

Nothing outstanding; approving.